Skip to content

Enable pdo-firebird nightly testing #12699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 17, 2023
Merged

Enable pdo-firebird nightly testing #12699

merged 3 commits into from
Nov 17, 2023

Conversation

KentarouTakeda
Copy link
Contributor

@KentarouTakeda KentarouTakeda commented Nov 17, 2023 •
edited
Loading

This pull request enables the pdo-firebird test provided in #12677 for nightly testing.

The solution is the same. nightly.yml uses test-linux/action.yml in the same way as push.yml to execute the test, so i created a firebird service container for all Jobs that reference the test action.

Note

Even after merging this pull request, some tests fail in nightly.yml. There are tests that are fine in push.yml but fail only in nightly.yml. You can see the results in my repository:

https://github.com/KentarouTakeda/php-src/actions/runs/6901762828/job/18777809789#step:12:151

We plan to create another pull request to resolve this issue.

UPDATE: 2023-11-17 23:29 JST

I also fixed a potential test fail exposed by running nightly.yml

Avoid memory leak report

Firebird's client library seems to have a memory leak issue. Most of the tests had the report suppressed, but there were two that were not specified and caused the test to fail. I fixed it. At that time, i unified the way to write suppression settings.

The memory leak problem was reported to Firebird by @SakiTakamachi with FirebirdSQL/firebird#7849 . If this is fixed, we should be able to unsuppress the report as well.

Memory alignment issue

There was a potential memory alignment violation when retrieving float values from the client library. I fixed it the same way as 21e0305.

@iluuu1994
Copy link
Member

We plan to create another pull request to resolve this issue.

Can you just add an --XLEAK-- to those files until the issue is resolved?

@KentarouTakeda
Copy link
Contributor Author

Added --XLEAK-- where required. All existing old writing styles have also been unified. There were some areas that required fixes other than memory leaks, so i also addressed those.

I will add it to the description later as the number of support items has increased.

@iluuu1994
Copy link
Member

@KentarouTakeda Your nightly test only showed three leaks:

  • ext/pdo_firebird/tests/bug_76452.phpt
  • ext/pdo_firebird/tests/gh10908.phpt
  • ext/pdo_firebird/tests/gh8576.phpt

Should it not be enough to add those?

@KentarouTakeda
Copy link
Contributor Author

@iluuu1994
Memory leaks occurred in all tests.
But it only had two (not three) reported.

The reason it wasn't reported elsewhere is because it was avoided using methods other than --XLEAK--. It's an old way of writing, so I unified it.

// before
--ENV--
LSAN_OPTIONS=detect_leaks=0

// after
--XLEAK--
A bug in firebird causes a memory leak when calling `isc_attach_database()`.
See https://github.com/FirebirdSQL/firebird/issues/7849

in addition to leaks, there were also areas that required modifications on the implementation side.

https://github.com/KentarouTakeda/php-src/actions/runs/6890531828/job/18743805627#step:12:269

This had the same response as the following fixes made in the past: 21e0305

Up to this point, I have finished working on another Pull Request that I was trying to create.

@iluuu1994
Copy link
Member

Ah sorry, I didn't look at it closely enough. That makes sense then.

@iluuu1994 iluuu1994 merged commit 7f7da6a into php:master Nov 17, 2023
@iluuu1994
Copy link
Member

Thank you @KentarouTakeda!

@KentarouTakeda KentarouTakeda deleted the testing-pdo-firebird-in-nightly branch November 17, 2023 14:49
@iluuu1994 iluuu1994 mentioned this pull request Jan 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

@TimWolla TimWolla Awaiting requested review from TimWolla TimWolla is a code owner

Assignees
No one assigned
Labels
Extension: pdo_firebird
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KentarouTakeda @iluuu1994